Use @fastify/fastify-postgres. CheckerNetwork/roadmap#220 #341

Goddhi · 2025-03-10T08:53:18Z

Added fastify and @fastify/postgres as dependencies
Updated stats/bin/spark-stats.js to use Fastify's PostgreSQL plugin
Improved connection error handling and graceful shutdown

EDIT @juliangruber:
For CheckerNetwork/roadmap#220

pyropy

Thank you for your submission @Goddhi 🚀

Good start, but there's some changes that need to be addressed:

Install @fastify/postgres package in correct workspace
Revert or rethink on how the migrations are done
Avoid creating new Fastify instances in order to register databases
Figure out how to perform queries from fastify handlers

pyropy · 2025-03-11T08:49:35Z

db/package.json

+    "@fastify/postgres": "^5.2.0",
+    "@fastify/url-data": "^6.0.3"


Suggested change

"@fastify/postgres": "^5.2.0",

"@fastify/url-data": "^6.0.3"

I think we should remove these packages from db workspace and install @fastify/postgres inside the stats workspace only. You can install workspace specific packages by running npm install @fastify/postgres -w stats

pyropy · 2025-03-11T08:50:55Z

stats/bin/migrate.js

+  EVALUATE_DB_URL
+} = process.env
+
+const app = Fastify({ logger: false })


I think that we shouldn't really create a new Fastify instance in order to run migrations. This should be done without the @fastify/postgres package.

pyropy · 2025-03-11T08:55:51Z

stats/bin/spark-stats.js


 const pgPools = await getPgPools()

+const dbFastify = Fastify({ logger: false })


I think we shouldn't create another Fastify instance here as we already do it inside createApp function. Rather, we should move registering the @fastify/postgres plugin and databases to be executed within createApp funciton.

pyropy · 2025-03-11T08:57:15Z

stats/bin/spark-stats.js

+
+await dbFastify.register(fastifyPostgres, {
+  connectionString: DATABASE_URL,
+  name: 'stats',


I think this is wrong because because we're instantiate only connection to stats database, but we do reference evaluate database bellow.

pyropy · 2025-03-11T08:58:31Z

stats/lib/db.js

+let fastifyApp = null
+let isInitialized = false


Suggested change

let fastifyApp = null

let isInitialized = false

I would avoid using globals in this case.

pyropy · 2025-03-11T08:59:03Z

stats/lib/db.js

+  if (isInitialized) return
+
+  // Create a minimal Fastify instance for database connections
+  fastifyApp = Fastify({ logger: false })


Instead of creating a new Fastify instance I think it would be better to pass down existing instance as function argument.

stats/lib/db.js

bajtos

+1 to what @pyropy wrote

Goddhi · 2025-03-13T23:07:53Z

@pyropy I do appreciate your reviews, i have made the following changes as requested, kindly look into thanks.
Moved @fastify/postgres dependencies to correct workspace
make the database connections to a single Fastify instance
removed the db.js which implements multiple Fastify instances. e.t.c.

pyropy

Let's try to avoid having dead code around. Also, let's try to use new way of accessing databases in the API handlers.

pyropy · 2025-03-17T11:53:29Z

stats/lib/app.js

+  const pgPools = {
+    stats: app.pg.stats,
+    evaluate: app.pg.evaluate,
+    async end() {
+      await app.close()
+    }
+  }


Suggested change

const pgPools = {

stats: app.pg.stats,

evaluate: app.pg.evaluate,

async end() {

await app.close()

}

}

I don't see this being used anywhere. If it's not used, let's removed. YAGNI

This is being passed to addRoutes() and addPlatformRoutes() below. However, instead of passing pgPools, the route handlers should access app.pg.stats etc. directly

pyropy · 2025-03-17T11:54:41Z

stats/bin/migrate.js

+const {
+  DATABASE_URL,
+  EVALUATE_DB_URL
+} = process.env


Suggested change

const {

DATABASE_URL,

EVALUATE_DB_URL

} = process.env

These constants seem not to be used anywhere. Let's delete them.

pyropy · 2025-03-17T11:55:28Z

stats/bin/migrate.js

+  console.log('Running migrations for stats database...')
+  const pgPools = await getPgPools()
+
+  // @ts-ignore - PgPoolStats actually does have a query method at runtime


Let's try to add correct types to migrate functions rather then using @ts-ignore. Using it should be avoided.

pyropy · 2025-03-17T11:55:43Z

stats/bin/migrate.js

+  // @ts-ignore - PgPoolStats actually does have a query method at runtime
+  await migrateStatsDB(pgPools.stats)
+
+  // @ts-ignore - Similarly for evaluate


Likewise, let's please not use @ts-ignore

stats/bin/migrate.js

pyropy · 2025-03-17T12:11:09Z

@Goddhi I suggest you link the issue you're working on in the PR description (and not in title). In this case you should've linked this issue CheckerNetwork/roadmap#220

stats/bin/migrate.js

stats/bin/spark-stats.js

juliangruber · 2025-03-18T12:43:51Z

stats/lib/app.js

+  const pgPools = {
+    stats: app.pg.stats,
+    evaluate: app.pg.evaluate,
+    async end() {
+      await app.close()
+    }
+  }


This is being passed to addRoutes() and addPlatformRoutes() below. However, instead of passing pgPools, the route handlers should access app.pg.stats etc. directly

Goddhi · 2025-03-19T08:05:13Z

@pyropy and @juliangruber have made commits that addresses the reviews.
I have updated route handlers to access database connections via request.server.pg which is a property from the @fastify/postgres and as well simplified my code by removing unnecessary abstractions as suggested.

juliangruber · 2025-03-19T08:09:54Z

stats/lib/platform-routes.js

+      const pgPools = adaptPgPools(request.server.pg);
+      reply.send(await fetchTopEarningParticipants(pgPools.stats, request.filter))    })


Suggested change

const pgPools = adaptPgPools(request.server.pg);

reply.send(await fetchTopEarningParticipants(pgPools.stats, request.filter)) })

reply.send(await fetchTopEarningParticipants(request.server.pg.stats, request.filter)) })

What do we need the adaptPgPools function for here?

juliangruber · 2025-03-19T08:10:51Z

stats/lib/routes.js

    app.addHook('onSend', filterOnSendHook)

    app.get('/deals/daily', async (/** @type {RequestWithFilter} */ request, reply) => {
+      const pgPools = adaptPgPools(request.server.pg);


what about instead changing the signature of fetchDaily... etc to accept request.server.pg? fetchDailyDealStats(request.server.pg, request.filter) etc

alright, noted, i will do that now

+1 on changing the signature and dropping the adaptPgPools function.

pyropy

You're making great progress @Goddhi! 🚀

pyropy · 2025-03-19T08:48:51Z

stats/lib/routes.js

    app.addHook('onSend', filterOnSendHook)

    app.get('/deals/daily', async (/** @type {RequestWithFilter} */ request, reply) => {
+      const pgPools = adaptPgPools(request.server.pg);


+1 on changing the signature and dropping the adaptPgPools function.

…ect directly

Goddhi · 2025-03-19T10:41:43Z

@juliangruber and @pyropy i have modified the fetcher functions signatures to accept Fastify's pg object directly and removed the adaptPgPools as suggested.

juliangruber · 2025-03-19T10:49:34Z

db/package.json

    ]
  }
-}
+}


Suggested change

}

}

ping @Goddhi

juliangruber · 2025-03-19T10:49:49Z

stats/bin/migrate.js

 const pgPools = await getPgPools()
 await migrateStatsDB(pgPools.stats)
-await migrateEvaluateDB(pgPools.evaluate)
+await migrateEvaluateDB(pgPools.evaluate)


Suggested change

await migrateEvaluateDB(pgPools.evaluate)

await migrateEvaluateDB(pgPools.evaluate)

juliangruber · 2025-03-19T10:50:10Z

stats/lib/app.js

+
+

Suggested change

db/package.json

juliangruber · 2025-03-19T10:51:47Z

stats/lib/platform-stats-fetchers.js


 /**
- * @param {Queryable} pgPool
+ * @param {Object} pg 


Suggested change

* @param {Object} pg

* @param {Queryable} pg

I believe this still follows the Queryable interface. Object is too unspecific.

stats/lib/stats-fetchers.js

juliangruber · 2025-03-20T10:49:42Z

stats/lib/platform-stats-fetchers.js

+
+/**
+ * @param {FastifyPg} pg - Fastify pg object with database connections
+ */


Suggested change

/**

* @param {FastifyPg} pg - Fastify pg object with database connections

*/

ping @Goddhi

juliangruber · 2025-03-20T10:50:09Z

stats/lib/stats-fetchers.js


 /**
- * @param {Object} pg 
+ * @param {any} pg - Fastify pg object with database connections


Suggested change

* @param {any} pg - Fastify pg object with database connections

* @param {FastifyPg} pg - Fastify pg object with database connections

ping @Goddhi

juliangruber · 2025-03-20T10:50:45Z

stats/lib/stats-fetchers.js

 /**
 * Fetches the retrieval stats summary for a single client for given date range.
- * @param {Object} pg
+ * @param {{stats: Queryable, evaluate: Queryable}} pg


should this not also be FastifyPg?

juliangruber · 2025-03-20T10:51:12Z

stats/lib/typings.d.ts

@@ -1,10 +1,16 @@
 import { FastifyRequest } from 'fastify'
+import { PostgresDb } from '@fastify/postgres'
+import { Queryable } from '@filecoin-station/spark-stats-db'


what is this type imported for?

juliangruber · 2025-03-20T10:52:03Z

@Goddhi CI is failing, please take a look

Goddhi · 2025-03-20T11:33:35Z

@Goddhi CI is failing, please take a look

FIXED

juliangruber · 2025-03-20T12:17:54Z

@Goddhi CI is still failing. You can run npm test locally to test this

Goddhi · 2025-03-20T13:01:54Z

@Goddhi CI is still failing. You can run npm test locally to test this

i have fixed other issues i found after running test, but the only issue left is this
standard: Use JavaScript Standard Style (https://standardjs.com)
/home/goddhi/spark-stats/stats/lib/platform-routes.js:2:3: 'fetchDailyStationCount' is defined but never used. (no-unused-vars)
i also observed this handler function is also not used /stats/lib/platform-routes.js of the main branch, i dont know if the CI will fail if i make commit now.

juliangruber · 2025-03-20T13:29:56Z

The function is used here:

spark-stats/stats/lib/platform-routes.js

Line 23 in 43792c3

reply.send(await fetchDailyStationCount(pgPools.evaluate, request.filter))

🤔

pyropy

Great progress 👍🏻

Let's clean the code up a bit; Let's try to pass database instance directly to the fetcher in which case we don't have to change fetcher signatures.

Also I'd advise you to format your code using standard as that's what we use in majority of our repositories. You can do it simply by running npx standard --fix.

pyropy · 2025-03-21T11:35:36Z

stats/bin/spark-stats.js

+  DATABASE_URL,
+  EVALUATE_DB_URL


Let's add default values for both env variables:

Suggested change

DATABASE_URL,

EVALUATE_DB_URL

DATABASE_URL = 'postgres://localhost:5432/spark_stats',

EVALUATE_DB_URL = 'postgres://localhost:5432/spark_evaluate'

pyropy · 2025-03-21T11:40:50Z

stats/lib/platform-routes.js

    })
    app.get('/stations/monthly', async (/** @type {RequestWithFilter} */ request, reply) => {
-      reply.send(await fetchMonthlyStationCount(pgPools.evaluate, request.filter))
+      reply.send(await fetchMonthlyStationCount(request.server.pg, request.filter))


Suggested change

reply.send(await fetchMonthlyStationCount(request.server.pg, request.filter))

reply.send(await fetchMonthlyStationCount(request.server.pg.evaluate, request.filter))

We could also pass correct instance of the database instead of passing the pg object. In that case we don't need to change fetchers.

This throws an error reason why went with only the pg object, besides the fetcher functions expects full pg object and uses pg.stats or pg.evaluate interally.

What error does it throw?

ping @Goddhi

pyropy · 2025-03-21T11:41:35Z

stats/lib/platform-stats-fetchers.js

 */
-export const fetchDailyStationCount = async (pgPool, filter) => {
-  const { rows } = await pgPool.query(`
+export const fetchDailyStationCount = async (pg, filter) => {


In case we pass correct database instance we don't need to change this handler.

ping @Goddhi

pyropy · 2025-03-21T11:42:29Z

stats/lib/routes.js

    })
    app.get('/retrieval-result-codes/daily', async (/** @type {RequestWithFilter} */ request, reply) => {
-      reply.send(await fetchDailyRetrievalResultCodes(pgPools, request.filter))
+      reply.send(await fetchDailyRetrievalResultCodes(request.server.pg, request.filter))


Suggested change

reply.send(await fetchDailyRetrievalResultCodes(request.server.pg, request.filter))

reply.send(await fetchDailyRetrievalResultCodes(request.server.pg.stats, request.filter))

We could also pass correct instance of the database instead of passing the pg object. In that case we don't need to change fetchers.

ping @Goddhi

Goddhi · 2025-03-23T17:40:25Z

@pyropy and @juliangruber i have made a commits regarding your suggestions, kindly help review it thanks.

juliangruber · 2025-03-24T08:09:44Z

stats/lib/platform-routes.js

    })
    app.get('/stations/monthly', async (/** @type {RequestWithFilter} */ request, reply) => {
-      reply.send(await fetchMonthlyStationCount(pgPools.evaluate, request.filter))
+      reply.send(await fetchMonthlyStationCount(request.server.pg, request.filter))


What error does it throw?

juliangruber · 2025-03-24T08:10:04Z

stats/lib/platform-stats-fetchers.js

+
+/**
+ * @param {FastifyPg} pg - Fastify pg object with database connections
+ */


ping @Goddhi

juliangruber · 2025-03-24T08:10:30Z

stats/lib/platform-stats-fetchers.js

 */
-export const fetchDailyStationCount = async (pgPool, filter) => {
-  const { rows } = await pgPool.query(`
+export const fetchDailyStationCount = async (pg, filter) => {


ping @Goddhi

juliangruber · 2025-03-24T08:10:45Z

stats/lib/routes.js

    })
    app.get('/retrieval-result-codes/daily', async (/** @type {RequestWithFilter} */ request, reply) => {
-      reply.send(await fetchDailyRetrievalResultCodes(pgPools, request.filter))
+      reply.send(await fetchDailyRetrievalResultCodes(request.server.pg, request.filter))


ping @Goddhi

juliangruber · 2025-03-24T08:11:20Z

@Goddhi tests are still failing, please run npm test to verify locally. See the CI failures here

Goddhi · 2025-03-25T06:32:58Z

@Goddhi tests are still failing, please run npm test to verify locally. See the CI failures here

Yeah the reason the tests are failing is becuase i need to modify the test files to sync with the changes made in the fetcher and route files, this is what i am currently doing.

Goddhi · 2025-03-25T06:37:25Z

#341 (comment)
@juliangruber this the errow that it throws request.server.pg.evaluate Argument of type 'PostgresDb' is not assignable to parameter of type 'FastifyPg'. Type 'PostgresDb' is not assignable to type 'Record<string, PostgresDb>'. Property 'pool' is incompatible with index signature. Type 'Pool' is missing the following properties from type 'PostgresDb': pool,

…lement test

bajtos · 2025-03-31T13:19:19Z

stats/test/app.test.js

-    await pgPools.stats.query('DELETE FROM daily_scheduled_rewards')
-    await pgPools.stats.query('DELETE FROM daily_reward_transfers')
-    await pgPools.stats.query('DELETE FROM daily_retrieval_result_codes')
+    await app.pg.evaluate.query('DELETE FROM retrieval_stats')


Let's preserve pgPools structure please so that we don't have to change every source code line in tests that is working with the DB.

For example:

before(async () => { app = await createApp({ // .. }) pgPools = app.pg }) // no changes are needed in individual tests

Alright, noted

stats/lib/platform-routes.js

bajtos · 2025-03-31T13:21:51Z

stats/lib/stats-fetchers.js

    FROM daily_allocator_retrieval_stats
    WHERE day >= $1 AND day <= $2
    GROUP BY allocator_id
   `, [


Can we keep the name pgPools to keep the diff smaller?

Suggested change

* @param {FastifyPg} pg - Fastify pg object with database connections

* @param {FastifyPg} pgPools - Fastify pg object with database connections

Same comment applies to all other similar places too.

Alright, noted

juliangruber · 2025-03-31T13:23:28Z

db/package.json

    ]
  }
-}
+}


ping @Goddhi

juliangruber · 2025-03-31T13:24:01Z

stats/bin/migrate.js

  migrateEvaluateDB,
  migrateStatsDB
 } from '@filecoin-station/spark-stats-db'
-


please undo this change to keep the diff clean

Alright, noted

juliangruber · 2025-03-31T13:24:36Z

stats/lib/platform-routes.js

    })
    app.get('/stations/monthly', async (/** @type {RequestWithFilter} */ request, reply) => {
-      reply.send(await fetchMonthlyStationCount(pgPools.evaluate, request.filter))
+      reply.send(await fetchMonthlyStationCount(request.server.pg, request.filter))


ping @Goddhi

juliangruber · 2025-03-31T13:25:00Z

stats/lib/routes.js

    })
    app.get('/retrieval-result-codes/daily', async (/** @type {RequestWithFilter} */ request, reply) => {
-      reply.send(await fetchDailyRetrievalResultCodes(pgPools, request.filter))
+      reply.send(await fetchDailyRetrievalResultCodes(request.server.pg, request.filter))


ping @Goddhi

juliangruber · 2025-03-31T13:25:15Z

stats/lib/stats-fetchers.js


 /**
- * @param {Object} pg 
+ * @param {any} pg - Fastify pg object with database connections


ping @Goddhi

…lement FIX: update to main

Goddhi · 2025-04-02T16:14:17Z

@pyropy Please can you help check what could be causing the CI to fail?
i have fixed the line trailing issues, now the issue seems to be this "AssertionError [ERR_ASSERTION]: INFLUXDB_TOKEN required"
i guess the token needs to be updated from the action secret. Thanks
@juliangruber please kindly help look into this

fastify-postgres--implementation

31a5f9b

Goddhi requested review from NikolasHaimerl, bajtos, juliangruber and pyropy as code owners March 10, 2025 08:53

pyropy requested changes Mar 11, 2025

View reviewed changes

bajtos requested changes Mar 12, 2025

View reviewed changes

fastify/postgress-implementation

6206573

Goddhi mentioned this pull request Mar 17, 2025

Migrate Spark API service to Fastify CheckerNetwork/spark-api#549

Open

pyropy requested changes Mar 17, 2025

View reviewed changes

juliangruber requested changes Mar 18, 2025

View reviewed changes

FIX:integrated fastify/postgress implementation into routes

51bfcad

Goddhi changed the title ~~fastify-postgres--implementationUse Use @fastify/fastify-postgres in our REST API services #220~~ https://github.com/CheckerNetwork/roadmap/issues/220 fastify-progres-implement Mar 19, 2025

Goddhi changed the title ~~https://github.com/CheckerNetwork/roadmap/issues/220 fastify-progres-implement~~ (CheckerNetwork/roadmap#220)[https://github.com/CheckerNetwork/roadmap/issues/220] Mar 19, 2025

juliangruber requested changes Mar 19, 2025

View reviewed changes

Goddhi changed the title ~~(CheckerNetwork/roadmap#220)[https://github.com/CheckerNetwork/roadmap/issues/220]~~ [CheckerNetwork/roadmap#220](https://github.com/CheckerNetwork/roadmap/issues/220) Mar 19, 2025

juliangruber changed the title ~~[CheckerNetwork/roadmap#220](https://github.com/CheckerNetwork/roadmap/issues/220)~~ Use @fastify/fastify-postgres in our REST API services. CheckerNetwork/roadmap#220 Mar 19, 2025

juliangruber mentioned this pull request Mar 19, 2025

Use @fastify/fastify-postgres in our REST API services CheckerNetwork/roadmap#220

Open

4 tasks

juliangruber changed the title ~~Use @fastify/fastify-postgres in our REST API services. CheckerNetwork/roadmap#220~~ Use @fastify/fastify-postgres. CheckerNetwork/roadmap#220 Mar 19, 2025

pyropy reviewed Mar 19, 2025

View reviewed changes

FIX: modified fetcher functiond signatures to accept Fastify's pg obj…

3717210

…ect directly

juliangruber requested changes Mar 19, 2025

View reviewed changes

Goddhi added 2 commits March 20, 2025 11:23

FIX: introduced FastifyPy type param

973b0da

Resolved merge conflicts in package-lock.json

826d075

juliangruber requested changes Mar 20, 2025

View reviewed changes

FIX: ci issue

e89a63c

FIX: added missing line of code

26d73c7

pyropy requested changes Mar 21, 2025

View reviewed changes

FIX: CI test issues

2608fdc

juliangruber requested changes Mar 24, 2025

View reviewed changes

Goddhi added 3 commits March 31, 2025 08:31

FEAT: test implementation for fastify/postgres

c0a95b2

Merge remote-tracking branch 'upstream/main' into fastify-progres-imp…

6a76461

…lement test

FIX: test implementation for fastify/postgres

f6a0319

bajtos requested changes Mar 31, 2025

View reviewed changes

juliangruber requested changes Mar 31, 2025

View reviewed changes

Goddhi added 3 commits April 2, 2025 05:19

FIX: changed naming convention to pgPOols and CI test

59448e4

Merge remote-tracking branch 'upstream/main' into fastify-progres-imp…

9423683

…lement FIX: update to main

FIX: linting issues

97ac2bd


		const pgPools = await getPgPools()

		const dbFastify = Fastify({ logger: false })

		const pgPools = adaptPgPools(request.server.pg);
		reply.send(await fetchTopEarningParticipants(pgPools.stats, request.filter)) })

	await migrateEvaluateDB(pgPools.evaluate)
	await migrateEvaluateDB(pgPools.evaluate)

	* @param {any} pg - Fastify pg object with database connections
	* @param {FastifyPg} pg - Fastify pg object with database connections

	reply.send(await fetchMonthlyStationCount(request.server.pg, request.filter))
	reply.send(await fetchMonthlyStationCount(request.server.pg.evaluate, request.filter))

	reply.send(await fetchDailyRetrievalResultCodes(request.server.pg, request.filter))
	reply.send(await fetchDailyRetrievalResultCodes(request.server.pg.stats, request.filter))

	* @param {FastifyPg} pg - Fastify pg object with database connections
	* @param {FastifyPg} pgPools - Fastify pg object with database connections

Use @fastify/fastify-postgres. CheckerNetwork/roadmap#220 #341

Are you sure you want to change the base?

Use @fastify/fastify-postgres. CheckerNetwork/roadmap#220 #341

Uh oh!

Conversation

Goddhi commented Mar 10, 2025 • edited by juliangruber Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pyropy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Goddhi commented Mar 13, 2025

Uh oh!

pyropy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pyropy commented Mar 17, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Goddhi commented Mar 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pyropy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Goddhi commented Mar 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Goddhi commented Mar 10, 2025 •

edited by juliangruber

Loading